Oops, accept is supposed to be async.
#157
Conversation
WalkthroughThis pull request introduces handshake synchronization in the web-transport-quiche driver. It adds a waker-based mechanism in 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-transport-quiche/src/ez/server.rs (1)
199-204:⚠️ Potential issue | 🟡 MinorStale doc comment — handshake is no longer complete at this point.
The doc says "with the TLS handshake already complete", but after this PR, the handshake completes inside
accept().await. TheIncomingis now created before the handshake finishes.Suggested doc fix
-/// A pre-accepted QUIC connection with the TLS handshake already complete. -/// -/// The peer address is available before calling [Incoming::accept]. +/// A pending QUIC connection whose TLS handshake may still be in progress. +/// +/// Call [Incoming::accept] to await handshake completion and obtain the [Connection]. +/// The peer address is available before calling [Incoming::accept].
🤖 Fix all issues with AI agents
In `@web-transport-quiche/src/server.rs`:
- Around line 160-168: The accept loop is blocked by awaiting
incoming.accept().await inside the select arm; instead create a FuturesUnordered
(e.g., tls_handshakes) and, when Some(incoming) = self.inner.accept() fires,
push a boxed/pinned future that performs incoming.accept().await into that
FuturesUnordered rather than awaiting inline; add a new select branch that polls
completed futures from tls_handshakes, and in that branch match the result and
on Ok(conn) push Box::pin(h3::Request::accept(conn)) into self.accept (on Err
log the warning), so TLS handshakes proceed concurrently with accepting new
connections and draining h3 handshakes.
🧹 Nitpick comments (1)
web-transport-quiche/src/ez/driver.rs (1)
85-104: Duplicate waker accumulation inhandshake_wakers.Each re-poll of
poll_handshakepushes a new clone of the waker without de-duplicating or replacing the previous entry. Spurious wakes (e.g., fromlocal.poll/remote.pollregistrations) will cause the same task's waker to accumulate. This is functionally harmless (extra wakes are idempotent), but slightly wasteful.A common pattern is to either clear the vec before pushing, or use a single
Option<Waker>if only one waiter is expected. Given that multiple tasks could legitimately wait (and the handshake completes once), theVecis reasonable, but you may want to guard against the same waker being pushed repeatedly from a single task.
web-transport-quiche/src/server.rs
Outdated
| Some(incoming) = self.inner.accept() => { | ||
| let conn = incoming.accept(); | ||
| self.accept.push(Box::pin(h3::Request::accept(conn))); | ||
| match incoming.accept().await { | ||
| Ok(conn) => { | ||
| self.accept.push(Box::pin(h3::Request::accept(conn))); | ||
| } | ||
| Err(err) => { | ||
| tracing::warn!("ignoring failed connection handshake: {}", err); | ||
| } | ||
| } |
There was a problem hiding this comment.
Head-of-line blocking: incoming.accept().await inside the select! arm stalls the accept loop.
The incoming.accept().await runs inside the body of the select! match arm. While it awaits the TLS handshake for one connection, the entire loop is blocked — no new QUIC connections can be accepted and no completed H3 handshakes can be processed. A slow or malicious client that delays its handshake will stall the server.
Consider spawning the TLS handshake into the existing FuturesUnordered (or a second one) so handshakes proceed concurrently, similar to how H3 handshakes are already handled:
Sketch of concurrent handshake approach
pub struct Server<M: ez::Metrics = ez::DefaultMetrics> {
inner: ez::Server<M>,
+ handshake: FuturesUnordered<BoxFuture<'static, Result<ez::Connection, ez::ConnectionError>>>,
accept: FuturesUnordered<BoxFuture<'static, Result<h3::Request, ServerError>>>,
}Then in the accept loop, push the TLS handshake future instead of awaiting it inline, and add a third select! branch to drain completed handshakes into the H3 accept pipeline:
Some(incoming) = self.inner.accept() => {
- match incoming.accept().await {
- Ok(conn) => {
- self.accept.push(Box::pin(h3::Request::accept(conn)));
- }
- Err(err) => {
- tracing::warn!("ignoring failed connection handshake: {}", err);
- }
- }
+ self.handshake.push(Box::pin(incoming.accept()));
+ }
+ Some(res) = self.handshake.next() => {
+ match res {
+ Ok(conn) => {
+ self.accept.push(Box::pin(h3::Request::accept(conn)));
+ }
+ Err(err) => {
+ tracing::warn!("ignoring failed connection handshake: {}", err);
+ }
+ }
}🤖 Prompt for AI Agents
In `@web-transport-quiche/src/server.rs` around lines 160 - 168, The accept loop
is blocked by awaiting incoming.accept().await inside the select arm; instead
create a FuturesUnordered (e.g., tls_handshakes) and, when Some(incoming) =
self.inner.accept() fires, push a boxed/pinned future that performs
incoming.accept().await into that FuturesUnordered rather than awaiting inline;
add a new select branch that polls completed futures from tls_handshakes, and in
that branch match the result and on Ok(conn) push
Box::pin(h3::Request::accept(conn)) into self.accept (on Err log the warning),
so TLS handshakes proceed concurrently with accepting new connections and
draining h3 handshakes.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-transport-quiche/src/ez/server.rs (1)
201-207:⚠️ Potential issue | 🟡 MinorStale doc comment: handshake is no longer already complete.
Line 201 says "with the TLS handshake already complete," but
Incomingis now yielded before the handshake finishes —accept()is what awaits it. Consider updating to something like: "A pre-accepted QUIC connection. Callaccept()to await TLS handshake completion."Proposed fix
-/// A pre-accepted QUIC connection with the TLS handshake already complete. +/// A pre-accepted QUIC connection. /// -/// The peer address is available before calling [Incoming::accept]. +/// The peer address is available immediately. +/// Call [Incoming::accept] to await TLS handshake completion.
1 participant
No description provided.